@FFMMM Given some of the complexity that arose from this PR I think there's some refactoring that may need to be done for requireOK + requireHttpStatusCodes.

Both functions take d time.Duration, resp *http.Response, e error which isn't great but it felt ergonomic to wrap doRequest like requireOK(c.doRequest). Note that d isn't used and non-nil e is simply returned back to the caller with the side effect of closing resp.

Currently require* can return 1) the transport-level error from doRequest; or 2) api.StatusError. These lines suggest to me there are cases where it's better to handle the error from doRequest (1) explicitly first before calling requireOK to extract api.StatusError (2), if any.

This would mean we'd have to make the handling a bit more verbose on all of the call sites, for example:

_, resp, err := c.doRequest(r)
if err != nil {
    return nil, fmt.Errorf("error making request: %w", err) // distinguish errors from doRequest
}
if err := requireOK(resp); err != nil {
    return nil, err         // this came from the response, meaning doRequest succeeded
}
defer closeResponseBody(resp)

Does this make sense? cc @dnephin

I haven't had a chance to catch up on this discussion, but I think that suggested refactor makes sense at a high level. Assuming the common case is requireOk we could even add a doRequestAndRequireOk that contains most of those lines. For the few places that don't call requireOk we can keep them as separate calls.

hey @kisunji and @dnephin thank y'all for chiming in! let me agree with y'all below with some context and proposed path:

I think there's some refactoring that may need to be done for requireOK + requireHttpStatusCodes.

Yes that's actually something that @markan and I were chatting about when looking at the api/agent.go code. It's weird ergonomics that the require() funcs close the response body for doRequest errors and not for other errors.

I think I'd love to have a chance to address this refactor in a subsequent PR.

Assuming the common case is requireOk we could even add a doRequestAndRequireOk that contains most of those lines.

There's actually >130 usages of requireOK() calls when I looked at that yesterday. I'd argue that a refactor that does doRequestAndRequireOk would be a big diff and even one that touches the internals of requireOK to modify behavior would warrant its own separate PR.

I'm hoping that we can merge this PR if it makes sense and I can take on the work to have a stab at refactoring the code in a subsequent PR. What do y'all think? 👍🏼 or 👎🏼 ?

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/461161.